Skip to content

perf: cache highlighter promises by key#136

Open
benvinegar wants to merge 1 commit intomainfrom
pi/todo-0b0340e2-highlighter-cache
Open

perf: cache highlighter promises by key#136
benvinegar wants to merge 1 commit intomainfrom
pi/todo-0b0340e2-highlighter-cache

Conversation

@benvinegar
Copy link
Copy Markdown
Member

Summary

  • cache in-flight/highlight-ready highlighter promises by ${appearance}:${language} in src/ui/diff/pierre.ts
  • keep the existing option cache, but make concurrent requests for the same key share one getSharedHighlighter() promise
  • drop failed promise entries from the cache so later calls can retry cleanly

Testing

  • bun run typecheck
  • bun test
  • bun test test/pierre.test.ts

This PR description was generated by Pi using OpenAI GPT-5

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 30, 2026

Greptile Summary

This PR introduces promise-level deduplication for the getSharedHighlighter call inside prepareHighlighter. Previously, concurrent renders for the same appearance:language key could each kick off their own getSharedHighlighter() call; now they all share a single in-flight Promise<Highlighter> stored in the new highlighterPromiseCache. Once the promise resolves, every subsequent caller receives the already-settled promise immediately, and on rejection the entry is evicted so the next caller gets a clean retry. The existing highlighterOptionsByKey option cache is preserved as-is.

Key changes:

  • Added type Highlighter = Awaited<ReturnType<typeof getSharedHighlighter>> for type-safe Map annotation.
  • Added highlighterPromiseCache: Map<string, Promise<Highlighter>> at module scope.
  • prepareHighlighter now returns the cached promise for in-flight and already-resolved entries, skipping option computation and a second getSharedHighlighter call.
  • A .catch() chained to getSharedHighlighter deletes the cache key before re-throwing, ensuring transient errors don't permanently poison the cache.

Confidence Score: 5/5

Safe to merge — the change is a clean promise-deduplication optimization with correct error-eviction semantics and no behavioral regressions.

No P0 or P1 issues found. The new cache correctly shares in-flight and resolved promises, the .catch() eviction prevents stale rejected entries, the highlighterOptionsByKey invariant is preserved, and the bounded key space (appearance × language) prevents unbounded cache growth. All edge cases (concurrent miss, concurrent hit before rejection, post-rejection retry) behave as intended.

No files require special attention.

Important Files Changed

Filename Overview
src/ui/diff/pierre.ts Adds a module-level highlighterPromiseCache Map that deduplicates concurrent getSharedHighlighter calls per ${appearance}:${language} key and evicts entries on rejection to allow clean retries.

Sequence Diagram

sequenceDiagram
    participant C1 as Caller A
    participant C2 as Caller B (concurrent)
    participant P as prepareHighlighter
    participant Cache as highlighterPromiseCache
    participant SH as getSharedHighlighter

    C1->>P: prepareHighlighter(lang, appearance)
    P->>Cache: get(cacheKey) → miss
    P->>SH: getSharedHighlighter(options)
    SH-->>P: Promise<Highlighter> (pending)
    P->>Cache: set(cacheKey, promise)
    P-->>C1: promise (pending)

    C2->>P: prepareHighlighter(lang, appearance)
    P->>Cache: get(cacheKey) → hit
    P-->>C2: same promise (pending)

    SH-->>C1: resolves → Highlighter
    SH-->>C2: resolves → Highlighter (shared)

    Note over Cache: Entry stays for future callers

    rect rgb(255, 220, 220)
        Note over SH,Cache: On failure path
        SH-->>P: rejects
        P->>Cache: delete(cacheKey)
        P-->>C1: rethrows error
        P-->>C2: rethrows error (same promise)
        Note over Cache: Entry cleared — next caller retries cleanly
    end
Loading

Reviews (1): Last reviewed commit: "perf: cache highlighter promises by key" | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant